Skip to content

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Oct 19, 2024

Run the full run-make test suite in the x86_64-gnu-debug CI job. This is currently the only CI job where //@ needs-force-clang-based-test will be satisfied, so some run-make tests will literally never be run otherwise. Before this PR, the CI job only ran run-make tests which contains the substring clang in its test name, which is both (1) a footgun because it's very easy to forget and (2) it masks tests that would otherwise fail (even failing to compile) because the test is skipped if doesn't have a clang in its test name.

With the environment of x86_64-gnu-debug, two run-make tests failed before this PR:

  1. tests/run-make/issue-84395-lto-embed-bitcode/rmake.rs: this was broken for a long time because objcopy in llvm bin tools was renamed to llvm-objcopy. This test was converted into a rmake.rs test, rather straight forward.
  2. tests/run-make/cross-lang-lto-riscv-abi/rmake.rs: this was broken for a long time and never worked. The old version inspected human-readable output of llvm-readobj --file-header looking for substring EF_RISCV_FLOAT_ABI_DOUBLE, but the human-readable output will only contain something like Flags: 0x5, RVC, double-float ABI, hence it will never match. This test was fixed by instead using the object crate to actually decode the ELF headers looking for the specific e_flags based on reading the RISCV ELF psABI docs.

This PR is best reviewed commit-by-commit, two commits setup the support library for functionality and two commits are for each of the failing run-make tests.

I had to bump the x86_64-gnu-debug job to be ran with a runner with larger disk space.

Part of #132034.

try-job: x86_64-gnu-debug

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Oct 19, 2024
@jieyouxu

This comment was marked as outdated.

@bors

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 19, 2024
[WIP] CI: try to drop `--test-args=clang` in `x86_64-gnu-debug`

Ok what if I open a separate PR...

r? `@ghost`

try-job: x86_64-gnu-debug
@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2024
@jieyouxu

This comment was marked as outdated.

@bors

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 20, 2024
[WIP] CI: try to drop `--test-args=clang` in `x86_64-gnu-debug`

Ok what if I open a separate PR...

r? `@ghost`

try-job: x86_64-gnu-debug
@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@jieyouxu

This comment was marked as outdated.

@bors

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2024
[WIP] CI: try to drop `--test-args=clang` in `x86_64-gnu-debug`

Ok what if I open a separate PR...

r? `@ghost`

try-job: x86_64-gnu-debug
@jieyouxu

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu

This comment was marked as outdated.

@bors

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2024
[WIP] CI: try to drop `--test-args=clang` in `x86_64-gnu-debug`

Ok what if I open a separate PR...

r? `@ghost`

try-job: x86_64-gnu-debug
@rust-log-analyzer

This comment has been minimized.

Full stage 2 build + run-make with full debug info seems to overwhelm
the standard 4c runner's storage capacity.
@jieyouxu
Copy link
Member Author

What if we downloaded more ram disk space
@bors try

@bors
Copy link
Collaborator

bors commented Oct 22, 2024

⌛ Trying commit 03c7f99 with merge 7b157b7...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 22, 2024
Run the full stage 2 `run-make` test suite in `x86_64-gnu-debug`

Run the full `run-make` test suite in the `x86_64-gnu-debug` CI job. This is currently the *only* CI job where `//@ needs-force-clang-based-test` will be satisfied, so some `run-make` tests will literally never be run otherwise. Before this PR, the CI job only ran `run-make` tests which contains the substring `clang` in its test name, which is both (1) a footgun because it's very easy to forget and (2) it masks tests that would otherwise fail (even failing to compile) because the test is skipped if doesn't have a `clang` in its test name.

With the environment of `x86_64-gnu-debug`, two `run-make` tests failed before this PR:

1. `tests/run-make/issue-84395-lto-embed-bitcode/rmake.rs`: this was broken for a long time because `objcopy` in llvm bin tools was renamed to `llvm-objcopy`. This test was converted into a rmake.rs test, rather straight forward.
2. `tests/run-make/cross-lang-lto-riscv-abi/rmake.rs`: this was broken for a long time and never worked. The old version inspected human-readable output of `llvm-readobj --file-header` looking for substring `EF_RISCV_FLOAT_ABI_DOUBLE`, but the human-readable output will only contain something like `Flags: 0x5, RVC, double-float ABI`, hence it will never match. This test was fixed by instead using the `object` crate to actually decode the ELF headers looking for the specific `e_flags` based on reading the RISCV ELF psABI docs.

This PR is best reviewed commit-by-commit, two commits setup the support library for functionality and two commits are for each of the failing `run-make` tests.

try-job: x86_64-gnu-debug
@bors
Copy link
Collaborator

bors commented Oct 22, 2024

☀️ Try build successful - checks-actions
Build commit: 7b157b7 (7b157b7bca4b26d4f40a49c7147d9e3ba8e472fe)

@jieyouxu jieyouxu removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Oct 22, 2024
@jieyouxu
Copy link
Member Author

try-job passed, so: @rustbot ready

Rolling two reviewers:

r? compiler (for tests and basic support lib changes themselves)
r? infra (for approval to modify runner type)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 22, 2024
@Kobzol
Copy link
Member

Kobzol commented Oct 22, 2024

+1 on the runner change. If more disk is needed, we don't really have a lot of other options than to just switch to a larger runner..

@petrochenkov petrochenkov removed their assignment Oct 23, 2024
@jieyouxu
Copy link
Member Author

@Kobzol this doesn't specifically need a compiler reviewer, if you think the tests look reasonable then I think it's fine

@Kobzol
Copy link
Member

Kobzol commented Oct 25, 2024

Ok! Nice to see one more makefile test removed.

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 25, 2024

📌 Commit 03c7f99 has been approved by Kobzol

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2024
@bors
Copy link
Collaborator

bors commented Oct 25, 2024

⌛ Testing commit 03c7f99 with merge b188577...

@bors
Copy link
Collaborator

bors commented Oct 25, 2024

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing b188577 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 25, 2024
@bors bors merged commit b188577 into rust-lang:master Oct 25, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 25, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b188577): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -0.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.1% [1.1%, 1.1%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.1% [-1.3%, -0.9%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.4% [-1.3%, 1.1%] 3

Cycles

Results (secondary -2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.1% [-2.1%, -2.0%] 4
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 788.765s -> 788.853s (0.01%)
Artifact size: 333.61 MiB -> 333.59 MiB (-0.00%)

@jieyouxu jieyouxu deleted the rmake-clang branch October 26, 2024 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants